Skip to content

fix(platform-wallet): auto_select_inputs honors Σ inputs == Σ outputs#3554

Open
Claudius-Maginificent wants to merge 80 commits into
v3.1-devfrom
fix/rs-platform-wallet-auto-select-inputs
Open

fix(platform-wallet): auto_select_inputs honors Σ inputs == Σ outputs#3554
Claudius-Maginificent wants to merge 80 commits into
v3.1-devfrom
fix/rs-platform-wallet-auto-select-inputs

Conversation

@Claudius-Maginificent
Copy link
Copy Markdown
Collaborator

@Claudius-Maginificent Claudius-Maginificent commented Apr 28, 2026

Issue

auto_select_inputs in packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs was inserting each selected address with its full balance as the input's Credits value, then returning as soon as accumulated covered output + fee. The address-funds-transfer protocol enforces Σ inputs.credits == Σ outputs.credits (strict equality), so a bank with ~500B credits funding a 50M output produced:

inputs  = { bank: 499_985_086_740 }
outputs = { target: 50_000_000 }
→ rejected: "Input and output credits must be equal"

Verified at rs-dpp/.../address_funds_transfer_transition/v0/state_transition_validation.rs and asserted on-chain by rs-drive-abci/.../address_funds_transfer/tests.rs::test_input_balance_decreased_correctly (new_balance == initial_balance - transfer_amount - fee).

Protocol semantics (now respected)

  • inputs[addr].credits = consumed amount from addr
  • outputs[addr] = credited amount to addr
  • Σ inputs.credits == Σ outputs.credits (strict equality)
  • Fee is deducted from the targeted input's remaining balance (post-consumption) per AddressFundsFeeStrategy. DeductFromInput(0) reduces the remaining balance by the fee — never the inputs map's Credits value
  • Every input must satisfy consumed >= min_input_amount (currently 100_000)

What changed

auto_select_inputs now respects the protocol contract:

  • Σ inputs.credits == Σ outputs.credits (strict equality) — selection trims so the inputs map sums to exactly total_output.
  • Fee headroom on DeductFromInput(0) target — the lex-smallest selected input retains at least estimated_fee of remaining balance.
  • min_input_amount enforcement — candidates below the floor are filtered upfront; Phase-4 distribution rolls sub-minimum residue into the fee target.
  • Output-address exclusion — addresses appearing in both candidate-input and transition-output sets are filtered out before selection (the canonical "self-funded transition" case that the protocol rejects).
  • Descending-balance candidate order — mirrors dash-evo-tool's allocator; collapses many cases to a single input.
  • InputSelection::Auto fee strategies supported: [DeductFromInput(0)] and [ReduceOutput(0)]. Any other shape returns a clear error redirecting to InputSelection::Explicit.
  • Defensive arithmeticchecked_add / saturating_sub on Credits throughout the selector path; overflow produces typed errors instead of wrapping.
  • Phase-3 retry on infeasibility — when fee_target_min > fee_target_max, the algorithm extends the prefix with the next candidate and retries instead of erroring out.

CI gap also closed in this PR: rs-platform-wallet was missing from .github/package-filters/rs-packages-no-workflows.yml, so Rust workspace tests had been skipping silently. The filter entry mirrors the existing pattern (path + *sdk alias). The previously-hidden Rust 1.92 clippy lints (in core_bridge.rs, wallet/apply.rs:316, tests/spv_sync.rs) are cleared in the same PR because they were blocking once CI started exercising the crate.

Commit history (initial 5-commit review wave for blame)

  1. aaf8be74 — initial Σ inputs == Σ outputs fix. Extracted the selection loop into a pure module-scope helper select_inputs that walks candidates and trims the result so the inputs map sums to exactly total_output.

  2. 9ea9e703 — fee-headroom guarantee at DeductFromInput(0) target (CodeRabbit critical). The original fix proved aggregate balance covered the fee but not that the specific fee-bearing input had remaining headroom. Now identifies the prospective fee target (lex-smallest of selected) and reserves at least estimated_fee of remaining balance on it.

  3. 687b1f86 — protocol-level reproduction test. Reconstructs the OLD buggy selector output for the CodeRabbit example, feeds the post-consumption input_current_balances through dpp::address_funds::fee_strategy::deduct_fee_from_outputs_or_remaining_balance_of_inputs, and asserts !fee_fully_covered. Proves the rejection at the protocol layer rather than asserting "the new output looks different."

  4. 60f7850a — sort candidates by balance descending (mirrors dash-evo-tool's allocator). Reduces the frequency of multi-input cases — when the largest single balance covers total_output + fee, the result is a 1-input map and the lex-smallest fee-target headroom logic doesn't fire at all. Bonus: fee_headroom_violation_errors now produces a debuggable error message.

  5. 9ff937ff — second review wave (4 blocking, 1 suggestion):

    • min_input_amount enforcement. auto_select_inputs filters candidates < min_input_amount upfront; Phase 4 distribution rolls any sub-minimum tail residue back into the fee target's consumption rather than producing an InputBelowMinimumError-prone tail.
    • fee_strategy restriction in transfer(). InputSelection::Auto now rejects any shape other than [DeductFromInput(0)] with a clear redirect to InputSelection::Explicit. The previous fallback path was publicly reachable but only protocol-correct for that single shape.
    • Phase 3 retry on infeasibility. When fee_target_min > fee_target_max, the algorithm extends the prefix with the next candidate and retries instead of erroring out — larger prefixes can yield a different lex-smallest fee target with sufficient headroom.
    • Early total_output < min_input_amount error (replaces the internal-error fallthrough).
    • Validator-based test assertions. assert_selection_validates helper builds an AddressFundsTransferTransitionV0 from each selector test's output and runs validate_structure. Catches future protocol-level regressions without depending on testnet.

CI / infrastructure (3 commits)

  1. 79c2b285ci(rs-packages-filter): trigger Rust workspace tests on rs-platform-wallet changes. The path filter at .github/package-filters/rs-packages-no-workflows.yml didn't list rs-platform-wallet, so any crate-only change there evaluated rs-packages = '[]' and Rust workspace tests silently skipped. This PR's prior 5 commits had never been validated by Rust CI — only by local cargo test. The filter entry mirrors the existing pattern (path + *sdk alias for transitive triggers).

  2. d610502 — merge v3.1-dev (9bd37f203a).

  3. 3c4f9199 — Rust 1.92 clippy hardening that the previously-skipped pipeline had been quietly accumulating:

    • field_reassign_with_default in core_bridge.rs::build_core_changeset → struct literal init
    • let_unit_value in wallet/apply.rs:316 (WalletInfoInterface::update_balance returns ()) → drop the let _ =
    • Stale core.chain.synced_height access in tests/spv_sync.rs → flattened to core.synced_height (struct shape changed upstream; the test never recompiled because workspace tests were skipping)

Tests (138 lib tests, all passing)

auto_select_tests module — 13 tests:

  • single_input_oversized_balance_trims_to_output_amount
  • descending_order_picks_single_largest_when_sufficient
  • pre_fix_buggy_selector_output_is_rejected_by_protocol_fee_deduction — protocol-level reproduction (asserts !fee_fully_covered)
  • fee_headroom_violation_errors
  • non_fee_target_below_min_input_redistributes
  • auto_select_inputs_excludes_output_addresses — confirms self-funded-transition filter
  • total_output_below_min_input_amount_errors
  • no_candidates_errors
  • reduce_output_happy_path_single_input — confirms [ReduceOutput(0)] strategy parity with [DeductFromInput(0)]
  • detect_no_selectable_inputs_combines_both_cases — pins the NoSelectableInputs variant fields (funded_outputs, sub_min_count, sub_min_aggregate)
  • augment_outputs_with_change_adds_residual_output
  • augment_outputs_with_change_rejects_duplicate_address
  • augment_outputs_with_change_rejects_no_surplus

Several assert structural validity via assert_selection_validatesAddressFundsTransferTransitionV0::validate_structure.

Note on withdrawal selector

The auto_select_inputs_for_withdrawal rustdoc clarifies the asymmetry: withdrawal validates Σ inputs > output_amount (strictly greater, surplus = fee), so its drain-everything strategy is correct by design. Not the same bug; no code change.

Verification

  • cargo fmt -p platform-wallet --check
  • cargo clippy --workspace --tests -- -D warnings
  • cargo test -p platform-wallet --lib138/138 passing
  • CI exercising the crate (path filter fixed in this PR)

Test plan

  • Unit tests cover trim invariant in single- and multi-input cases
  • Regression test for fee-only-tail-input case
  • Protocol-level reproduction proves the pre-fix output is rejected by deduct_fee_from_outputs_or_remaining_balance_of_inputs
  • Validator-based test assertions on 5 selector tests
  • Signatures stable — transfer(), auto_select_inputs, select_inputs unchanged. Backward-compatible additions only: new WalletError::NoSelectableInputs struct variant (replacing the prior generic insufficient-balance message) and InputSelection::Auto now accepts [ReduceOutput(0)] in addition to [DeductFromInput(0)]. Downstream exhaustive matches on WalletError need updating.
  • Workspace clippy clean under Rust 1.92
  • Rust workspace tests now triggered by path filter
  • Live e2e re-run on testnet via test(platform-wallet): e2e framework + full test suite — triage pins, Found-*/PA-* guards, fail-closed persist, Stage-2 merge #3549 once both merge

Provenance

Originally surfaced and fixed during work on PR #3549 (rs-platform-wallet e2e harness). Split out so the production-code fix can ship independently of the long-running e2e branch. Subsequent commits address review feedback from CodeRabbit and thepastaclaw reviewers and close a CI coverage gap that was hiding pre-existing breaks on v3.1-dev.

🤖 Generated with Claude Code

Triage wave (11 commits, 2026-05-21)

Applied 9 of 11 triaged findings from a fresh grumpy-review + comment-verification pass at HEAD d5d2b3d271. 2 deferred as out-of-scope TODOs.

Commit Finding Sev Summary
098484f34f QA-001 HIGH augment_outputs_with_change now rejects change_amount < min_output_amount via typed ChangeBelowMinimumOutput; platform_version plumbed; (0, min_output_amount) band test added
8ee78a4f60 CMT-001 LOW InputSelection::Auto rustdoc rewritten to current contract (balance-desc, dust filter, output-address exclusion, supported fee strategies, typed errors)
3aeed87bfa CMT-002 LOW is_empty_no_records() now includes addresses_derived, matching canonical CoreChangeSet::is_empty()
6c3fb02b0c QA-003 MED Defensive balance-descending sort in select_inputs_reduce_output and select_inputs_deduct_from_input; invariant documented
e2140bd475 QA-004 MED Post-Phase-4 fee recompute against selected.len() instead of prefix.len(); eliminates false "insufficient funds" on objectively spendable wallets
b85b6a9a90 QA-005 LOW OnlyOutputAddressesFunded now carries sub_min_count + sub_min_aggregate to preserve dust diagnostics under the combined case
cfe3c33247 QA-006 LOW checked_sum_credits + InputSumOverflow for caller-supplied input maps in transfer_with_change_address; trusted call sites still saturate
9854e38674 QA-002 MED (breaking) outputs now typed as IndexMap<PlatformAddress, Credits> so caller controls "output 0" insertion order; [ReduceOutput(0)] + Some(change_addr) rejects when change_addr is lex-smaller than every user output
9c136cd912 CMT-003 LOW New FFI codes ErrorOnlyOutputAddressesFunded = 13 and ErrorOnlyDustInputs = 14; Swift PlatformWalletResultCode + typed PlatformWalletError mirrored; table-driven Rust test pins routing
7c7a2ec585 QA-007 / CMT-004 LOW TODO markers for two deferred follow-ups (pre-existing read-snapshot/broadcast race; missing balance-hydration regression test)
2957623756 cargo fmt --all cleanup

Triage decisions

  • Fixed (9): QA-001, QA-002, QA-003, QA-004, QA-005, QA-006, CMT-001, CMT-002, CMT-003
  • Deferred (2): QA-007 (pre-existing race, out of PR scope), CMT-004 (test gap on file outside PR diff)

Verification

  • cargo fmt --all --check
  • cargo clippy -p platform-wallet -p platform-wallet-ffi --all-targets -- -D warnings
  • cargo test -p platform-wallet --lib144 passing
  • cargo test -p platform-wallet-ffi --lib75 passing (incl. new typed_errors_route_to_dedicated_codes)

Known follow-ups (out of PR scope)

  • DPP AddressFundsTransferTransition outputs are still BTreeMap at the chain layer; the IndexMap is a wallet-only affordance. A doc comment on the DPP transition struct cross-referencing the wallet semantics would help future readers — separate PR.
  • FFI parse_outputs overwrites duplicate-address entries silently. Pre-existing behavior, but worth a sanity rejection in a follow-up.

🤖 Co-authored by Claudius the Magnificent AI Agent

`auto_select_inputs` in `wallet/platform_addresses/transfer.rs` was
inserting each selected address with its FULL balance as the input's
`Credits` value, then returning as soon as accumulated covered
`output + fee`. With a bank holding ~500B credits and a 50M output, the
SDK got `inputs = {bank: 499_985_086_740}, outputs = {target: 50_000_000}`
and the protocol rejected it because address-funds-transfer enforces
`Σ inputs.credits == Σ outputs.credits` (strict equality, verified at
`rs-dpp/.../address_funds_transfer_transition/v0/state_transition_validation.rs`,
asserted on-chain by
`rs-drive-abci/.../address_funds_transfer/tests.rs::test_input_balance_decreased_correctly`,
which checks `new_balance == initial_balance - transfer_amount - fee`).

The protocol's actual semantics:
- `inputs[addr].credits` = consumed amount from `addr`
- `outputs[addr]` = credited amount to `addr`
- `Σ inputs.credits == Σ outputs.credits`
- Fee is deducted from the targeted input's REMAINING balance (post-
  consumption) per `AddressFundsFeeStrategy`. `DeductFromInput(0)`
  reduces the *remaining balance* by the fee — never the inputs map's
  `Credits` value.

Fix: extract the selection loop into a pure module-scope helper
`select_inputs(candidates, outputs, total_output, fee_strategy,
platform_version)` that:

1. Walks candidates in DIP-17 order, tentatively appending each to a
   `Vec<(address, balance)>` to drive the per-iteration fee estimate.
2. Stops when `accumulated >= total_output + estimated_fee` (the
   accumulated balance must cover the fee from the last input's
   remaining balance).
3. Builds the returned map front-to-back, consuming each input in
   insertion order until exactly `total_output` is reached. Inputs
   added solely to satisfy the per-input fee margin are excluded
   from the final map — preserving Σ inputs.credits == total_output
   without violating `min_input_amount`.

Side benefits:
- The pure helper is unit-testable without constructing a full
  `PlatformWalletManager` + `PlatformAddressWallet`. Five tests cover
  the fix:
    - `single_input_oversized_balance_trims_to_output_amount`
    - `two_input_selection_trims_only_the_last`
    - `fee_only_tail_input_does_not_inflate_input_sum` (regression for
      the Σ-inputs-greater-than-Σ-outputs case raised in Copilot review)
    - `insufficient_balance_errors`
    - `no_candidates_errors`
- The full per-`PlatformAddressWallet` async method `auto_select_inputs`
  now just gathers `(address, balance)` candidates and calls
  `select_inputs`, which keeps the testability win without changing
  public API.

Doc note in `auto_select_inputs_for_withdrawal` clarifies the
asymmetry: withdrawal validates `Σ inputs > output_amount` (strictly
greater, surplus = fee), so its drain-everything strategy is correct
by design — NOT the same bug as the transfer selector. No code
change there.

Verification:
- `cargo check --tests -p platform-wallet`             OK
- `cargo clippy --tests -p platform-wallet -- -D warnings` OK
- `cargo fmt -p platform-wallet`                        OK
- `cargo test -p platform-wallet --lib`                 115/115

Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
@github-actions github-actions Bot added this to the v3.1.0 milestone Apr 28, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 28, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Restricts auto input-selection to a single fee-strategy shape and replaces the greedy selector with a deterministic, tested pure selector; plus small refactors and test/adaptations and a package-filter addition.

Changes

Cohort / File(s) Summary
Input selection & tests
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs
Restricts InputSelection::Auto to the single supported fee-strategy [DeductFromInput(0)]; replaces previous greedy ascending selector with snapshot/filter → sort (descending) → pure select_inputs helper that returns per-address consumed credits summing to total_output, preserves fee headroom for the lexicographically-smallest DeductFromInput(0) target, folds sub-min_input_amount tail into fee target, and exposes estimate_fee_for_inputs_pub. Adds comprehensive unit and protocol-layer regression tests.
Core changeset projection
packages/rs-platform-wallet/src/changeset/core_bridge.rs
Rewrites build_core_changeset projection to compute new_utxos/spent_utxos first and return a constructed CoreChangeSet literal instead of mutating a default via field reassignments.
Restore/apply behavior
packages/rs-platform-wallet/src/wallet/apply.rs
Calls self.core_wallet.update_balance() for side effects (no returned changeset used); updates inline docs to reflect update_balance returns () and to avoid clippy let_unit_value.
SPV sync test update
packages/rs-platform-wallet/tests/spv_sync.rs
Updates RecordingPersister::store to read synced_height from flattened CoreChangeSet (c.synced_height) instead of nested chain.synced_height, adjusting test persistence metadata observation.
Package filter addition
.github/package-filters/rs-packages-no-workflows.yml
Adds new package filter group rs-platform-wallet matching packages/rs-platform-wallet/** and including *sdk dependency set.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

Poem

🐰 I hopped through balances, tails in a row,
Picked the fattest coins so fees still could grow,
Folded tiny crumbs into the pot,
Wrote tests that thumped — no corner forgot,
A tiny rabbit happy to watch ledgers flow.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main bug fix: it specifies the component (auto_select_inputs), describes the core issue (honoring the sum equivalence property Σ inputs == Σ outputs), and directly addresses the central problem fixed in the PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/rs-platform-wallet-auto-select-inputs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@thepastaclaw
Copy link
Copy Markdown
Collaborator

thepastaclaw commented Apr 28, 2026

✅ Review complete (commit 30e612b)

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- Around line 341-365: When building the final `selected` map after `accumulated
>= required`, ensure the address referenced by the fee strategy
`DeductFromInput(index)` is reserved the estimated fee before returning: locate
the `chosen` prefix and the loop that constructs `selected` using
`remaining`/`total_output`, compute the required fee headroom for the
fee-bearing input (from `fee_strategy`/`DeductFromInput`) and reduce that
input's available amount by that fee (i.e., instead of consuming up to
`remaining`, cap consumption so the fee-bearing input keeps at least
`estimated_fee`), and if that causes the input to be insufficient, continue
selecting additional candidates or return an error; update uses of `selected`,
`remaining`, `accumulated`, and `required` accordingly so the returned map
guarantees the fee-bearing input still has the reserved fee.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 56d75786-5d1c-44b8-a304-2e962f372120

📥 Commits

Reviewing files that changed from the base of the PR and between 844edba and aaf8be7.

📒 Files selected for processing (1)
  • packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs

Comment thread packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs Outdated
@lklimek lklimek changed the title fix(rs-platform-wallet): auto_select_inputs honors Σ inputs == Σ outputs fix(wallet): auto_select_inputs honors Σ inputs == Σ outputs Apr 28, 2026
@lklimek lklimek changed the title fix(wallet): auto_select_inputs honors Σ inputs == Σ outputs fix(platformwallet): auto_select_inputs honors Σ inputs == Σ outputs Apr 28, 2026
@lklimek lklimek changed the title fix(platformwallet): auto_select_inputs honors Σ inputs == Σ outputs fix(platform-wallet): auto_select_inputs honors Σ inputs == Σ outputs Apr 28, 2026
…arget

CodeRabbit caught a critical bug on PR #3554's `select_inputs`: the helper
ensured `Σ inputs.credits == Σ outputs.credits` (the protocol's structural
invariant) but did NOT ensure that the address targeted by
`DeductFromInput(0)` had post-consumption remaining balance >= the
estimated fee.

Worked example from CodeRabbit:
  candidates    = [(addr_a, 20M), (addr_b, 50M)]   // addr_a < addr_b lex
  total_output  = 30M
  fee_strategy  = [DeductFromInput(0)]
  Old result    = {addr_a: 20M, addr_b: 10M}       // Σ matches; addr_a drained
  Drive applies DeductFromInput(0) over inputs sorted by key (BTreeMap order),
  hitting addr_a — whose remaining balance is 0 — so `min(fee, 0) = 0`,
  `fee_fully_covered = false`, validator rejects with
  AddressesNotEnoughFundsError.

The Wave-8 single-input live e2e accidentally avoided this because the
fee target had ~1B credits left over after consumption — multi-input
auto-selected transfers would have hit it on first contact.

This rewrite:

- Phase 1 (unchanged): pick smallest DIP-17-ordered prefix covering
  total_output + estimated_fee.
- Phase 2: identify the fee target = lex-smallest address in the prefix
  (= `BTreeMap` index 0, what `DeductFromInput(0)` will hit per
   `rs-dpp/src/address_funds/fee_strategy/.../v0/mod.rs`).
- Phase 3: consume the *minimum* allowed amount from the fee target
  (`max(min_input_amount, total_output − Σ other balances)`) so it
  retains the most remaining balance for fee deduction. Error out
  with a descriptive AddressOperation if even that minimum leaves
  less than `estimated_fee` remaining.
- Phase 4: distribute the rest of `total_output` across the other
  prefix entries in DIP-17 order.
- Phase 5: defensive invariant checks.

`min_input_amount` is fetched from
`platform_version.dpp.state_transitions.address_funds.min_input_amount`
(currently 100k across v1/v2/v3 of platform-version).

For non-`[DeductFromInput(0)]` fee strategies the helper falls back to
the previous "consume from front" distribution that only enforces the
Σ invariant — none of the wallet's call sites use anything else today.

Tests:
- updated `two_input_selection_trims_only_the_last` →
  `two_input_selection_keeps_fee_headroom_at_index_zero` to assert the
  new distribution AND the headroom invariant.
- updated `fee_only_tail_input_does_not_inflate_input_sum`'s expected
  outputs (the tail is no longer dropped — it absorbs the consumption
  the fee target sheds).
- added `fee_target_keeps_remaining_for_fee_deduction` (CodeRabbit's
  exact scenario, with the headroom invariant as the load-bearing
  assertion).
- added `fee_headroom_violation_errors` (lex-smallest address too
  small to retain headroom → descriptive error rather than transition
  the validator will reject).
- `single_input_oversized_balance_trims_to_output_amount`,
  `insufficient_balance_errors`, `no_candidates_errors` pass unchanged.

`cargo test -p platform-wallet --lib` → 117 / 117 green
`cargo clippy -p platform-wallet --tests -- -D warnings` → clean
`cargo fmt -p platform-wallet --check` → clean

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- Around line 441-461: The distribution loop that sets each consumed = min(bal,
remaining) can insert inputs below min_input_amount (e.g., consumed <
min_input_amount); after that loop (the one that inserts entries into prefix and
uses variables fee_target_consumed, remaining, consumed, fee_target_addr),
validate all non-zero consumed values against min_input_amount and either (a)
rebalance by moving consumption from fee_target_consumed or other large entries
to bump small entries up to min_input_amount while preserving total consumption,
or (b) return an error indicating distribution impossible; implement the
simplest correct choice for your flow (prefer returning an error if safe
redistribution is complex) and ensure the function returns early on failure so
subsequent debug_asserts aren’t relied on.
- Around line 349-353: The code currently silently falls back to front-trimming
for any AddressFundsFeeStrategy other than the single-item [DeductFromInput(0)]
inside transfer()/where InputSelection::Auto is handled; change this to reject
unsupported auto-selection fee strategies by returning a clear error instead of
performing the unsafe fallback. Locate the branch handling InputSelection::Auto
in transfer.rs (the block that examines fee_strategy and falls back to
front-trimming) and add a guard that checks the strategy sequence—if it is not
exactly the single DeductFromInput(0) pattern, return an Err (with a descriptive
enum/variant or mapped error) indicating unsupported fee strategy for
auto-selection so callers cannot produce inputs that sum to outputs but will
fail on-chain once fees are applied. Ensure the new error flows out of
transfer() consistently with existing error types.
- Around line 363-385: The loop that builds the DIP-17-ordered prefix (variables
prefix, accumulated, covered) currently breaks out as soon as accumulated >=
required, which prevents trying larger prefixes when Phase 3 (fee target
feasibility using fee_target_min/fee_target_max and DeductFromInput(0)
semantics) fails; change the logic so that when a covering prefix is found you
run Phase 3 checks but do not return/error on Phase 3 failure—continue the for
(address, balance) in candidates iteration (calling estimate_fee_for_inputs_pub
as before) to grow the prefix and try later candidates until either Phase 3
succeeds or all candidates are exhausted, only then set covered/error
accordingly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 857ac9b6-556f-4771-a503-d3c0069a9ecb

📥 Commits

Reviewing files that changed from the base of the PR and between aaf8be7 and 9ea9e70.

📒 Files selected for processing (1)
  • packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs

Comment thread packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs Outdated
Comment thread packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs Outdated
Comment thread packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs Outdated
lklimek and others added 2 commits April 28, 2026 09:54
…ee-headroom bug

Adds `pre_fix_buggy_selector_output_is_rejected_by_protocol_fee_deduction`
to the `select_inputs` test module. Reconstructs the exact `inputs` map
the pre-fix `auto_select_inputs` would have returned for CodeRabbit's
example (candidates (20M, 50M), total_output 30M, `DeductFromInput(0)`),
runs the post-consumption remaining balances through the live dpp
fee-deduction code path, and asserts `fee_fully_covered == false` —
i.e. the protocol rejects it with `AddressesNotEnoughFundsError`.

Distinct from `fee_target_keeps_remaining_for_fee_deduction`, which
asserts the new selector's output meets the headroom invariant. This
reproduction proves the bug at the protocol layer rather than merely
asserting "the new output looks different" — it would have stayed red
without the fix in 9ea9e70.

Verification:
- cargo check --tests -p platform-wallet                 OK
- cargo clippy --tests -p platform-wallet -- -D warnings OK
- cargo fmt -p platform-wallet                           OK
- cargo test -p platform-wallet --lib                    118/118

Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
…descending

Internal-only change to `auto_select_inputs`. Candidates were
previously collected in DIP-17 derivation index order; now they
sort by balance descending before being handed to `select_inputs`.

Mirrors the dash-evo-tool allocator
(`src/ui/wallets/send_screen.rs:155-157`). Effects:

- Single largest balance covering `total_output + estimated_fee`
  => 1-input result, no multi-input case, no lex-smallest fee
  headroom logic firing. Common path simplified.
- Multi-input cases (when the largest alone isn't enough) still
  go through the headroom-respecting distribution introduced in
  9ea9e70 — unchanged, still correct.
- No public API change. `transfer()`, `auto_select_inputs`,
  `select_inputs` signatures all identical.

Adds `descending_order_picks_single_largest_when_sufficient` to
the existing test module to lock in the common-path behavior.
Other tests pass candidates directly to `select_inputs` and are
order-agnostic by design — unchanged.

The `fee_headroom_violation_errors` error message now includes
the fee-target address, its balance, required headroom, and
remaining-after-consumption to ease debugging.

Verification:
- cargo check --tests -p platform-wallet                 OK
- cargo clippy --tests -p platform-wallet -- -D warnings OK
- cargo fmt -p platform-wallet                           OK
- cargo test -p platform-wallet --lib                    119/119

Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (2)

442-453: Fallback path for non-[DeductFromInput(0)] strategies still risks on-chain rejection.

The docstring at lines 371-375 acknowledges this limitation, but the code silently proceeds with a distribution that only guarantees Σ inputs == Σ outputs without reserving fee headroom on the actual fee-bearing input. If transfer() is ever called with a different fee strategy (e.g., DeductFromInput(1) or multi-step strategies), the returned inputs map could still fail on-chain when the targeted input lacks remaining balance for fee deduction.

A previous review suggested returning an error for unsupported strategies. The current approach documents the limitation but doesn't prevent misuse. Consider whether rejecting unsupported strategies is preferable to silent fallback with potential on-chain failure.

Alternative: Reject unsupported strategies explicitly
     if !single_deduct_from_input_zero {
-        let mut selected: BTreeMap<PlatformAddress, Credits> = BTreeMap::new();
-        let mut remaining = total_output;
-        for (addr, bal) in prefix.iter() {
-            if remaining == 0 {
-                break;
-            }
-            let consumed = (*bal).min(remaining);
-            selected.insert(*addr, consumed);
-            remaining = remaining.saturating_sub(consumed);
-        }
-        return Ok(selected);
+        return Err(PlatformWalletError::AddressOperation(
+            "Auto input selection currently supports only [DeductFromInput(0)] fee strategy. \
+             Other strategies require explicit input selection.".to_string(),
+        ));
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs` around
lines 442 - 453, The fallback branch that builds `selected` when
`!single_deduct_from_input_zero` must not silently return a transfer that may
fail on-chain; instead make `transfer()` validate the fee strategy up-front and
return an explicit error for unsupported strategies (e.g., when the strategy is
not `FeeStrategy::DeductFromInput(0)` or `single_deduct_from_input_zero` is
false). Replace the current loop-return branch that constructs `selected` with
an Err variant (create or reuse a `TransferError::UnsupportedFeeStrategy` or
similar), and ensure callers handle that error; keep the `prefix`-consumption
logic only for the supported `single_deduct_from_input_zero` path.

405-409: Phase 1 early break may cause false "fee headroom" failures when a larger prefix would succeed.

A previous review noted that because DeductFromInput(0) targets the lex-smallest address (not the first in iteration order), a later candidate joining the prefix can become the new fee target and make an otherwise-infeasible selection work. The current code breaks at the first covering prefix without checking Phase 3 feasibility.

With the descending-balance sort, this scenario is less common (the first candidates are the largest balances), but it can still occur when:

  1. The largest-balance address is also lex-smallest
  2. That address has just enough to cover total_output but not total_output + fee
  3. Adding a lex-smaller address would shift the fee-target role to a smaller-balance address with better headroom characteristics

Consider continuing to accumulate candidates until Phase 3 succeeds or all candidates are exhausted, rather than erroring immediately on the first Phase 3 failure.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs` around
lines 405 - 409, The early break on the first prefix that meets accumulated >=
required causes false failures because Phase 3 (fee-target reassignment via
DeductFromInput(0)) may succeed for a larger prefix; in the loop that checks
accumulated, required and sets covered=true then break, remove the immediate
break and instead, after accumulated >= required, invoke the Phase 3 feasibility
check (the same logic that uses DeductFromInput(0) / the fee-target selection)
and only set covered=true and stop accumulating if that Phase 3 check succeeds;
if Phase 3 fails, continue accumulating more candidates and only error or mark
covered=false after all candidates are exhausted. Ensure you update the
variables used in the Phase 3 check to reflect the extended prefix and keep the
existing semantics for setting covered and returning an error when no prefix
passes Phase 3.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- Around line 509-521: The loop that distributes `remaining` across `prefix` may
insert entries below `min_input_amount` because it uses `consumed =
bal.min(remaining)` without validating per-input minimum; update the Phase 4
loop (the block iterating `for (addr, bal) in prefix.iter()`) to only insert a
`consumed` value if `consumed >= min_input_amount`, otherwise do not insert that
address and instead add the small remainder to the `fee_target_addr` (or
accumulate it to be merged into the fee target while preserving any fee
headroom) so no non-fee-target input can be below `min_input_amount`; retain
existing `remaining`/`selected` semantics and keep the Phase 5 `debug_assert`
intact.

---

Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- Around line 442-453: The fallback branch that builds `selected` when
`!single_deduct_from_input_zero` must not silently return a transfer that may
fail on-chain; instead make `transfer()` validate the fee strategy up-front and
return an explicit error for unsupported strategies (e.g., when the strategy is
not `FeeStrategy::DeductFromInput(0)` or `single_deduct_from_input_zero` is
false). Replace the current loop-return branch that constructs `selected` with
an Err variant (create or reuse a `TransferError::UnsupportedFeeStrategy` or
similar), and ensure callers handle that error; keep the `prefix`-consumption
logic only for the supported `single_deduct_from_input_zero` path.
- Around line 405-409: The early break on the first prefix that meets
accumulated >= required causes false failures because Phase 3 (fee-target
reassignment via DeductFromInput(0)) may succeed for a larger prefix; in the
loop that checks accumulated, required and sets covered=true then break, remove
the immediate break and instead, after accumulated >= required, invoke the Phase
3 feasibility check (the same logic that uses DeductFromInput(0) / the
fee-target selection) and only set covered=true and stop accumulating if that
Phase 3 check succeeds; if Phase 3 fails, continue accumulating more candidates
and only error or mark covered=false after all candidates are exhausted. Ensure
you update the variables used in the Phase 3 check to reflect the extended
prefix and keep the existing semantics for setting covered and returning an
error when no prefix passes Phase 3.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3b42e9a8-3597-407f-9ca0-e9a419a38732

📥 Commits

Reviewing files that changed from the base of the PR and between 9ea9e70 and 60f7850.

📒 Files selected for processing (1)
  • packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

PR fixes the original Σ inputs == Σ outputs bug, but introduces three new protocol violations. The selector reasons about fee headroom in DIP-17 insertion order while the chain applies DeductFromInput(i) over BTreeMap key order — combined with dropping/draining tail inputs, this leaves the actual fee-bearing input with no remaining balance. The trim can also produce inputs below min_input_amount (100_000). The new tests assert only the structural invariant and would not catch any of these regressions.

Reviewed commit: aaf8be7

🔴 2 blocking | 🟡 1 suggestion(s) | 💬 2 nitpick(s)

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [BLOCKING] lines 321-365: Selector reserves fee headroom in insertion order, but the protocol charges DeductFromInput(0) against the BTreeMap-first input
  `select_inputs` keeps `chosen` in insertion (DIP-17) order and consumes from front to back, fully draining every input except the last to reach exactly `total_output`. It returns a `BTreeMap`, however, and the on-chain validator resolves `DeductFromInput(index)` via `remaining_balances.iter().nth(index)` (verified at `rs-dpp/.../state_transition_estimated_fee_validation.rs:48-69`), i.e. against BTreeMap key order — the lex-smallest selected address.

When the BTreeMap-first address is not the same as the insertion-order tail, all fee headroom ends up on the wrong input. The new test `two_input_selection_trims_only_the_last` demonstrates the failure mode directly: `addr_a = [0x01;20]` (BTreeMap key 0) is consumed for its full 20M balance — remaining = 0; `addr_b` keeps 40M of headroom. With `DeductFromInput(0)` the protocol charges the fee to `addr_a`, which has 0 left, so `fee_fully_covered = false` and the transition is rejected with `AddressesNotEnoughFundsError` (`rs-drive-abci/.../validate_fees_of_event/v0/mod.rs:209-224`).

The `fee_only_tail_input_does_not_inflate_input_sum` test exposes the same root cause via dropping rather than draining: `addr_a` has `total_output + 1` and is consumed for `total_output`, leaving 1 credit of remaining balance on the only returned input — far below any realistic transfer fee. The aggregate guarantee `Σ remaining ≥ fee` is irrelevant because the protocol charges the fee from one specific input, not the aggregate.

The helper must guarantee that the input the protocol will actually charge (BTreeMap-first when `fee_strategy = [DeductFromInput(0)]`) retains at least `estimated_fee` of remaining balance after consumption. Practical options: (a) compute the BTreeMap-first selected address up front and direct the trim residue to it, leaving headroom there; (b) restrict consumption on the fee-target input to `balance − estimated_fee`. The unit tests need to assert remaining balances after consumption, not just `Σ values == total_output`, to catch this class of bug.
- [BLOCKING] lines 351-365: Trimmed tail input can fall below `min_input_amount`, causing structural rejection
  The trim sets `consumed = (*bal).min(remaining)` per input, with no lower bound. The transfer validator rejects any input below `platform_version.dpp.state_transitions.address_funds.min_input_amount` (currently 100_000) with `InputBelowMinimumError` (`rs-dpp/.../address_funds_transfer_transition/v0/state_transition_validation.rs:146-167`).

A perfectly normal multi-input scenario triggers this: candidates A=5M, B=100M with `total_output = 5_050_000` yields `{A: 5_000_000, B: 50_000}`, and 50_000 < 100_000. Or two 100k inputs funding a 150k output: `{first: 100_000, second: 50_000}` — also rejected. None of the new tests exercise this boundary.

Fix options: filter candidates with `balance >= min_input_amount` at the call site and either skip a tail that would trim below the minimum (rolling residue back into a prior input) or refuse to add a candidate whose post-trim consumption would violate the limit. The helper aims to encode protocol correctness, so this should be enforced inside `select_inputs` rather than left to the validator.
- [SUGGESTION] lines 412-539: Tests assert the input-sum invariant only, not full state-transition validation
  `single_input_oversized_balance_trims_to_output_amount`, `two_input_selection_trims_only_the_last`, and `fee_only_tail_input_does_not_inflate_input_sum` all assert only that `Σ values == total_output`. They never assemble the result into an `AddressFundsTransferTransition` and run it through `validate_structure` + the fee-validation pipeline. As a result they pass on inputs maps that the on-chain validator deterministically rejects (see the two blocking findings above). The dpp validators are pure and don't need a live node — wiring an integration assertion into these tests would catch both classes of regression and is the right substitute for the still-pending testnet rerun referenced in the PR description.

Comment thread packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs Outdated
Comment thread packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs Outdated
Comment thread packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs Outdated
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

The DeductFromInput(0) headroom bug from the prior review is fixed: the selector now targets the BTreeMap-first input via lex-smallest selection, and the new pre_fix_buggy_selector_output_is_rejected_by_protocol_fee_deduction test exercises dpp's actual fee-deduction path. Two prior blockers remain reachable on the current head, however: Phase 4's distribution across non-fee-target inputs has no min_input_amount floor, and the public-API fallback for fee strategies other than [DeductFromInput(0)] produces input maps the validator deterministically rejects.

Reviewed commit: 60f7850

🔴 2 blocking | 🟡 1 suggestion(s) | 💬 2 nitpick(s)

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [BLOCKING] lines 504-521: Non-fee-target inputs can still be trimmed below `min_input_amount`
  Phase 3 pins `fee_target_min ≥ min_input_amount` (line 484), but Phase 4 then distributes `total_output − fee_target_consumed` across the non-fee-target prefix entries with `let consumed = (*bal).min(remaining)` and inserts any positive value (lines 516-520) — there is no per-input lower bound. The validator at `packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/v0/state_transition_validation.rs:157-167` iterates `self.inputs.values()` and rejects ANY input below `min_input_amount` (100_000) with `InputBelowMinimumError`.

A construction that survives Phase 1 and Phase 3 but trips the validator:
- candidates after balance-desc sort: `[(addr_X=0x01, 1_000_000), (addr_Y=0x02, 30_000)]`
- `total_output = 950_000`, `min_input_amount = 100_000`
- Phase 1 needs both inputs (e.g. `fee_for_1 = 60_000` → 1_000_000 < 1_010_000; `fee_for_2 = 80_000` → 1_030_000 ≥ 1_030_000 ✓)
- `fee_target = addr_X`, `fee_target_max = 920_000`, `other_total = 30_000`, `fee_target_min = max(100_000, 920_000) = 920_000` — 920_000 ≤ 920_000 ✓
- Phase 4: `remaining = 30_000` → `addr_Y` inserted as 30_000 < 100_000 → rejected.

Fix options: filter candidates with `balance < min_input_amount` at the call site; refuse to insert a non-fee-target consumption that would land below the minimum (rolling residue back to the fee target, which already has remaining headroom); or bail out with a descriptive error. The existing `fee_headroom_violation_errors` test exercises only the fee-target min-input path; an analogous test for the tail input would catch this.
- [BLOCKING] lines 442-454: Fallback path is reachable via public API and produces protocol-invalid input maps
  `transfer()` (line 31) is `pub` and accepts an arbitrary `fee_strategy: AddressFundsFeeStrategy` from any caller. With `InputSelection::Auto`, this routes through `select_inputs()`, which only implements protocol-correct logic for the exact shape `[DeductFromInput(0)]` (line 437-440). For every other strategy, lines 442-453 fall back to a front-consume distribution that guarantees only `Σ inputs == total_output` and ignores both fee-target headroom and `min_input_amount`.

Reachable failure: with `fee_strategy = [DeductFromInput(1)]`, candidates `[(addr_b, 20M), (addr_a, 50M)]` where `addr_a < addr_b`, and `total_output = 30M`, the fallback returns `{addr_b: 20M, addr_a: 10M}`. The protocol resolves `DeductFromInput(i)` against BTreeMap key order (`packages/rs-dpp/src/address_funds/fee_strategy/.../v0/mod.rs`), so index 1 points at `addr_b`, which is fully drained — fee deduction fails exactly like the original bug. `ReduceOutput(...)` strategies can produce structurally invalid trailing inputs for the same reason.

The doc on lines 371-375 acknowledges this as 'must be revisited if [strategy] changes', but the public API surface is wide open today. Either constrain the strategy at the entry point, return an explicit `Err` for unsupported shapes, or extend the fee-target/min-input logic to general strategies. Returning a known-suspect map silently is the riskier option — it forces a future caller to stumble into the same protocol rejection that motivated this PR.
- [SUGGESTION] lines 484-545: `total_output < min_input_amount` falls through to misleading 'Internal selection error'
  When `total_output < min_input_amount` (e.g. caller asks to transfer 50_000 credits with min_input=100_000), the 1-input path computes `fee_target_min = max(min_input_amount, total_output) = 100_000 > total_output`, so `selected = {addr: 100_000}` and `input_sum = 100_000 ≠ total_output`. Phase 4's loop runs once with `remaining = total_output.saturating_sub(100_000) = 0`, then the flow trips the `debug_assert_eq!` at line 527 in debug builds and falls through to the line-538 'Internal selection error' branch in release.

The protocol disallows any transfer with `total_output < min_input_amount` (no input set can satisfy both `Σ inputs == total_output` and per-input `≥ min_input_amount`). This deserves an early, descriptive error like 'Transfer amount X below minimum Y' rather than the internal-error path that's documented as 'should never trip'. Add an early check at the top of `select_inputs` (or in the `transfer` entry-point on `outputs.values().sum()`).

Comment thread packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs Outdated
Comment thread packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs Outdated
Comment thread packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs Outdated
Comment thread packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs Outdated
…egy, retry on Phase 3 fail

Addresses the second wave of review findings on PR #3554:

1. [BLOCKING] Phase 4 distribution no longer produces inputs below
   `min_input_amount`. `auto_select_inputs` now filters candidates
   with `balance < min_input_amount` upfront — they cannot legally
   appear in the inputs map. In Phase 4, when a non-fee-target
   tail entry would consume less than `min_input_amount`, the
   residue rolls back into the fee target's consumption (which has
   surplus headroom by construction). Returns a descriptive error
   if rollback would violate the fee-target headroom invariant.

2. [BLOCKING] `transfer()` rejects unsupported `fee_strategy`
   shapes for `InputSelection::Auto`. Auto-select currently only
   implements protocol-correct logic for `[DeductFromInput(0)]`;
   any other strategy returns `PlatformWalletError::AddressOperation`
   with a clear message redirecting callers to
   `InputSelection::Explicit`. Explicit paths still accept
   arbitrary strategies (caller's responsibility).

3. [BLOCKING] When Phase 3 (`fee_target_min > fee_target_max`) fails
   in `select_inputs`, the algorithm now extends the prefix with the
   next candidate and retries instead of erroring out. Larger
   prefixes may yield a different lex-smallest fee target with
   sufficient headroom. Errors out only when candidates are
   exhausted and no covering prefix is feasible.

4. [SUGGESTION] `select_inputs` returns an early descriptive error
   when `total_output < min_input_amount` — the protocol forbids
   this regardless of input shape, so an explicit error beats the
   internal "should never trip" branch that some callers were
   reaching.

5. [SUGGESTION] Existing selector tests now also build a minimal
   `AddressFundsTransferTransitionV0` and run `validate_structure`,
   asserting protocol-level validity in addition to the
   `Σ inputs == total_output` invariant. Catches future regressions
   without needing a live node.

Coderabbit findings DUuz (#3554), DUu1 (#3554), E5L5 (#3554),
thepastaclaw findings F9fo, GMHz, GMH5, GMH_, F9fv addressed.
Outdated F9fk references the renamed test from before 9ea9e70.
Nitpicks F9fz/GMID/F9f5/GMIH deferred (unreachable / low value).

Verification:
- cargo check --tests -p platform-wallet                  OK
- cargo clippy --tests -p platform-wallet -- -D warnings  OK
- cargo fmt -p platform-wallet                            OK
- cargo test -p platform-wallet --lib                     121/121

Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (1)

607-631: Phase 5 invariant checks are partially release-protected.

The debug_assert statements at lines 611-624 verify critical protocol invariants:

  1. Σ inputs == total_output — also checked in release at line 626 ✓
  2. Fee target is BTreeMap index-0 (lex-smallest)
  3. Fee target retains ≥ estimated_fee remaining balance
  4. Every input ≥ min_input_amount

Invariants 2-4 are only asserted in debug builds. If any of these fail in release (due to a future regression in Phase 1-4 logic), the transition would be submitted and rejected by the protocol layer rather than caught here.

Given the algorithm's structure and test coverage, these should never trip. However, promoting invariant 3 (the fee headroom check) to a release-mode error would provide defense-in-depth for the exact bug this PR fixes.

♻️ Optional: Add release-mode check for fee headroom
     if input_sum != total_output {
         return Err(PlatformWalletError::AddressOperation(format!(
             "Internal selection error: Σ inputs ({}) != total_output ({})",
             input_sum, total_output
         )));
     }
+
+    let fee_target_remaining = fee_target_balance.saturating_sub(fee_target_consumed);
+    if fee_target_remaining < estimated_fee {
+        return Err(PlatformWalletError::AddressOperation(format!(
+            "Internal selection error: fee target {} remaining {} < estimated fee {}",
+            format_address(&fee_target_addr),
+            fee_target_remaining,
+            estimated_fee,
+        )));
+    }
 
     Ok(selected)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs` around
lines 607 - 631, The fee-headroom check currently uses debug_assert! on
fee_target_balance.saturating_sub(fee_target_consumed) >= estimated_fee, which
is only active in debug builds; replace that debug-only assertion with a
release-mode runtime check inside the same scope (where selected,
fee_target_balance, fee_target_consumed and estimated_fee are available) so that
if the condition fails you return
Err(PlatformWalletError::AddressOperation(...)) with a clear message including
the computed headroom and required estimated_fee; keep the other debug_asserts
as-is and ensure the new check mirrors the existing error style used for
input_sum != total_output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- Around line 607-631: The fee-headroom check currently uses debug_assert! on
fee_target_balance.saturating_sub(fee_target_consumed) >= estimated_fee, which
is only active in debug builds; replace that debug-only assertion with a
release-mode runtime check inside the same scope (where selected,
fee_target_balance, fee_target_consumed and estimated_fee are available) so that
if the condition fails you return
Err(PlatformWalletError::AddressOperation(...)) with a clear message including
the computed headroom and required estimated_fee; keep the other debug_asserts
as-is and ensure the new check mirrors the existing error style used for
input_sum != total_output.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e663ab0e-42e2-48de-b207-cd8e4fa9740b

📥 Commits

Reviewing files that changed from the base of the PR and between 60f7850 and 9ff937f.

📒 Files selected for processing (1)
  • packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs

lklimek and others added 6 commits April 28, 2026 16:33
…allet changes

Adds `rs-platform-wallet` as a filter entry in
`.github/package-filters/rs-packages-no-workflows.yml`. Without this,
crate-only changes under `packages/rs-platform-wallet/` evaluate to
`rs-packages = '[]'` and the `rs-workspace-tests` job in
`.github/workflows/tests.yml` gates off — meaning the crate's unit
tests never run in CI when only that crate is touched.

This gap surfaced on PR #3554 itself: five commits, 121 unit tests,
none of them executed by `Rust workspace tests` (all reported as
SKIPPED). Local `cargo test -p platform-wallet --lib` was the only
validation. Reviewers seeing "all green" could miss that the actual
Rust validation was skipped.

The filter entry mirrors the existing pattern: list the crate path
and inherit the SDK alias (`*sdk`) so transitive SDK changes also
trigger workspace tests for the wallet, matching how `wasm-sdk` and
`rs-sdk-ffi` are wired.

Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
…now run)

The CI filter addition in 79c2b28 made `Rust workspace tests` run
on `rs-platform-wallet` for the first time in a while, surfacing
three pre-existing breaks that the silently-skipped pipeline had
been accumulating:

1. `src/changeset/core_bridge.rs` (`build_core_changeset`) —
   `field_reassign_with_default` lint. `let mut cs =
   CoreChangeSet::default(); cs.new_utxos = ...; cs.spent_utxos = ...;`
   replaced with a struct literal carrying the derived values plus
   `..CoreChangeSet::default()` for forward-compat fields.

2. `src/wallet/apply.rs:316` — `let_unit_value` lint.
   `WalletInfoInterface::update_balance` returns `()`; the `let _ =
   ...` discards a unit value. Calling the method directly is the
   intended shape.

3. `tests/spv_sync.rs:74-78` — stale field access. The integration
   test still walked `core.chain.synced_height` even though
   `CoreChangeSet` was flattened (see existing rustdoc on
   `synced_height` direct field). Replaced with
   `core.synced_height` directly.

None of these are bugs — clippy hardening and a stale test field
that `cargo test --lib` never compiled. Verified:

- `cargo clippy --workspace --tests -- -D warnings` clean
- `cargo clippy -p platform-wallet --tests -- -D warnings` clean
- `cargo test -p platform-wallet --lib` 121/121

Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
… work

Apply claudius:coding-best-practices rules: length cap (<=2 preferred,
3 mediocre), present-state only (no Wave/PR-number history), two-tier
(strict for internal, liberal for public API rustdoc).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…o-select

Extends transfer() / auto_select_inputs to accept [ReduceOutput(0)] in addition
to [DeductFromInput(0)]. Output 0 absorbs the fee, so input selection skips the
fee-headroom reservation. Σ inputs == Σ outputs invariant preserved via last-
input trim.

5 new tests in auto_select_tests cover happy path, multi-input trim, multi-
output isolation, output-too-small error, and structural validation.

Resolves PR #3549 thread r-aCky's production prerequisite.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.15%. Comparing base (9f3bfa3) to head (1fba51c).

Additional details and impacted files
@@             Coverage Diff              @@
##           v3.1-dev    #3554      +/-   ##
============================================
- Coverage     87.16%   87.15%   -0.01%     
============================================
  Files          2607     2606       -1     
  Lines        319420   319221     -199     
============================================
- Hits         278413   278215     -198     
+ Misses        41007    41006       -1     
Components Coverage Δ
dpp 87.67% <ø> (-0.01%) ⬇️
drive 85.95% <ø> (ø)
drive-abci 89.60% <ø> (ø)
sdk ∅ <ø> (∅)
dapi-client ∅ <ø> (∅)
platform-version ∅ <ø> (∅)
platform-value 92.17% <ø> (ø)
platform-wallet ∅ <ø> (∅)
drive-proof-verifier 49.16% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…allet-auto-select-inputs

# Conflicts:
#	packages/rs-platform-wallet/src/changeset/core_bridge.rs
@lklimek lklimek added the ready for final review Ready for the final review. If AI was involved in producing this PR, it has already had a reviewer. label Apr 30, 2026
lklimek and others added 2 commits April 30, 2026 10:08
…educeOutput Phase 4

Annotates `select_inputs_reduce_output`'s Phase 4 fee-headroom check
to document the known dpp-layer bug (platform #3040) where
`estimate_min_fee` models only the static state_transition_min_fees
floor and excludes storage + processing costs. For small `output[0]`,
the auto-selector greenlights selections that then fail on-chain with
AddressesNotEnoughFundsError. Comment-only — no behaviour change.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek and others added 11 commits May 21, 2026 16:04
QA-001: `transfer_with_change_address` computed `change_amount = input_sum
− user_output_sum` and unconditionally inserted it. The protocol enforces
a per-output minimum (`OutputBelowMinimumError`, code 10810) via
`platform_version.dpp.state_transitions.address_funds.min_output_amount`,
so a residual in the `(0, min_output_amount)` band would ship a transition
the chain rejects.

Plumb `platform_version` through `augment_outputs_with_change`, add a
typed `PlatformWalletError::ChangeBelowMinimumOutput { change_amount,
min_output_amount }` variant, and reject the band before broadcast. Adds
the missing `(0, min_output_amount)` band test alongside the existing
residual=0 / residual=55M coverage.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CMT-001: the public `InputSelection::Auto` rustdoc described the
pre-PR contract ("consume addresses from lowest derivation index
upward"). The current implementation orders candidates
balance-descending, drops dust below `min_input_amount`, excludes
output-collision addresses, restricts fee strategies to
`[DeductFromInput(0)]` / `[ReduceOutput(0)]`, and surfaces typed
`OnlyOutputAddressesFunded` / `OnlyDustInputs` errors. Rewrite the
docstring to match.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CMT-002: the adapter-side shortcut treated a changeset carrying only
derived addresses as empty and dropped it before reaching the
persister, while the canonical `CoreChangeSet::is_empty()` counts
`addresses_derived` as persisted state. Align the two definitions so
no future derived-address-only flow silently loses its event.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ding

QA-003: both `select_inputs_deduct_from_input` and
`select_inputs_reduce_output` rely on a balance-descending invariant
(`select_inputs_reduce_output`'s Phase-2 trim and Phase-3 donor-lift
treat the last entry as the smallest; `select_inputs_deduct_from_input`
grows the smallest covering prefix). Production callers pre-sort via
`build_auto_select_candidates`, but direct test / future callers would
silently miscompute. Re-sort defensively at the top of each selector and
document the invariant; updates two tests whose fixtures relied on the
caller's accidental order.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QA-004: `select_inputs_deduct_from_input` sized `estimated_fee` against
`prefix.len()` in Phase 1, but the Phase-4 residue-fold can leave
`selected.len() < prefix.len()` when sub-minimum tail entries collapse
into the fee target. The post-residue headroom recheck then over-bounded
the fee target's max consumption and could falsely reject feasible
selections as "Cannot satisfy fee headroom".

Recompute `estimated_fee` against the actual selected input count and
re-derive `fee_target_consumed` from `total_output − Σ other selected`.
Adds a 3-entry/2-selected regression test alongside the existing
sub-minimum redistribution coverage.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QA-005: when both "funded-but-also-output" and "dust" diagnostics
applied, `detect_no_selectable_inputs` returned `OnlyOutputAddressesFunded`
without any breadcrumb to the simultaneous dust shape. A UI rotating
the receive address on the first error would hit `OnlyDustInputs` on
the second try with no prior warning.

Add `sub_min_count` and `sub_min_aggregate` fields to the variant; the
detector now populates them in the combined case so the caller can
surface both shapes in one go. Output-collision still wins the primary
classification (more actionable fix), the dust info is auxiliary.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QA-006: `transfer_with_change_address` accepts the input map verbatim
from the caller under `Explicit` / `ExplicitWithNonces`. The shared
`saturating_sum_credits` helper silently saturated to `u64::MAX` on a
bogus FFI sum, muddying the downstream protocol diagnostic.

Introduce `checked_sum_credits` (try-fold + `Credits::checked_add`)
and route the two caller-supplied input sums through it. Saturating
helper stays for wallet-derived sums where the supply invariant is
trusted. Adds a typed `PlatformWalletError::InputSumOverflow` variant.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_addr

QA-002: outputs were a `BTreeMap<PlatformAddress, Credits>`, so under
`[ReduceOutput(0)]` "output 0" was always the lex-smallest key — never
first-inserted. A change address that happened to be lex-smaller than
every user output would silently absorb the fee instead of the
caller-declared target. Σ invariants held; UX expectations did not.

Switch the public `PlatformAddressWallet::transfer` and
`transfer_with_change_address` outputs parameter to
`indexmap::IndexMap<PlatformAddress, Credits>` so caller insertion
order is preserved (useful for debugging and UI), and document the
lex-key constraint DPP imposes at the chain layer. Reject the
`[ReduceOutput(0)] + Some(change_addr)` case where `change_addr` is
lex-smaller than every user output, since that is the only path where
the surprise can ship a transition the caller didn't intend.

Mirror the change at the FFI boundary: `parse_outputs` returns
`IndexMap`, with the FFI call-site array order surviving the boundary.

BREAKING CHANGE: `PlatformAddressWallet::transfer` and
`transfer_with_change_address` now take `indexmap::IndexMap` for the
outputs / user_outputs parameter instead of `std::collections::BTreeMap`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CMT-003: `From<PlatformWalletError>` collapsed every variant to
`ErrorUnknown`, so Swift callers building typed errors from
`result.code` lost the distinction between
`OnlyOutputAddressesFunded` and `OnlyDustInputs` introduced upstream.
That defeats the entire reason the typed Rust variants exist.

Add `ErrorOnlyOutputAddressesFunded` (= 13) and `ErrorOnlyDustInputs`
(= 14) to `PlatformWalletFFIResultCode`, route the `From` impl, and
mirror in Swift: new `PlatformWalletResultCode` cases, `init(ffi:)`
arms via the cbindgen `PLATFORM_WALLET_FFI_RESULT_CODE_*` constants,
and typed `PlatformWalletError` cases. Adds a table-driven Rust test
covering both new variants plus the catch-all fallback.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QA-007 (defer): annotate the auto-select snapshot site with the
pre-existing read-snapshot vs broadcast race; the new headroom math
leans harder on the snapshot but fixing the race is out of this PR's
scope.

CMT-004 (defer): annotate `initialize_from_persisted` with a TODO
noting the missing direct regression test for restart-time balance
hydration; track separately.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cosmetic only — `cargo fmt --all` over the touched file.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lklimek lklimek requested a review from shumkov as a code owner May 21, 2026 15:58
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Carried-forward prior findings: none. Three prior issues are fixed in 295762375685bb1df67b6f961acf77b99abbdf4b, and the remaining balance-hydration regression-test gap is explicitly deferred with a TODO rather than resolved. New findings in the latest delta: one blocking Swift transfer bug and two Rust/FFI correctness issues remain.

🔴 1 blocking | 🟡 2 suggestion(s)

3 additional finding(s) omitted (not in diff).

Review details

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift`:
- [BLOCKING] lines 281-300: Swift still targets the change output by insertion index even though Rust/DPP reduce outputs by lexicographic key order
  This wrapper appends change last and then sends `ReduceOutput(change_index)` using `ffiOutputs.count - 1`, which assumes the chain will treat the last appended output as the fee target. That assumption is false in the current Rust implementation: `PlatformAddressWallet::transfer()` converts the caller-facing `IndexMap` into a `BTreeMap`, and its own docs now state that output index 0 is determined by lexicographic address order, not insertion order. A change address that sorts before any recipient will therefore become DPP's reduced output, so the fee is deducted from the wrong entry and a recipient can receive less than the amount Swift promised. Rust added a guard for this in `transfer_with_change_address`, but this Swift path bypasses that API and reintroduces the same bug at the FFI boundary.

In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [SUGGESTION] lines 121-154: Transfer persistence records external recipient addresses as wallet-owned state
  `transfer_address_funds` proves and returns address info for the full `inputs ∪ outputs` set, not just wallet-owned inputs. This loop writes every returned `P2pkh` into the local account with `set_address_credit_balance`, and `ManagedPlatformAccount::contains_platform_address()` treats any address present in `address_balances` as belonging to the account. When the proved address is not actually in the wallet's derived-address pool, the code still persists it with `address_index = 0`; on restore, `insert_persisted_entry()` inserts that row back into the account state as if it were an owned address. A single transfer to an external address can therefore poison later balance/account views and persisted wallet state with addresses the wallet cannot sign for.

In `packages/rs-platform-wallet-ffi/src/platform_address_types.rs`:
- [SUGGESTION] lines 203-205: `parse_outputs` silently coalesces duplicate destination addresses instead of rejecting them
  The FFI parser still feeds each output through `map.insert(addr, entry.balance)`, so non-Rust callers can pass the same destination address multiple times and only the last balance survives. The Rust API cannot express duplicate keys, which makes this parser the only place to reject them before the wallet builds a different output set than the caller supplied. That mismatch is now more dangerous because callers may compute change or validate `Σ inputs == Σ outputs` from the original array while the wallet broadcasts the deduplicated map.

Reviewed commit: 29576237

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

I verified the carried-forward findings against the checked-out head 94bbb6453d7dce30e828d14214827d4f9f5db2aa. All three still apply in the current source: the Swift explicit-change wrapper still targets ReduceOutput by insertion index, transfer persistence still records non-derived recipient addresses into persisted wallet-owned state, and the FFI output parser still silently overwrites duplicate destination rows. Earlier issues around InputSelection::Auto documentation, CoreChangeSet::is_empty_no_records(), and typed FFI error routing are fixed on this SHA; the restore-hydration regression test is still intentionally deferred and documented with TODO(CMT-004).

Reviewed commit: 94bbb645

🔴 1 blocking | 🟡 2 suggestion(s)

These are carried-forward findings revalidated on the current head; GitHub could not map them cleanly to fresh inline diff comments, so I’m posting the details here.

1. Blocking: Swift explicit-change transfers still target the fee reduction by insertion index instead of DPP output order

packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:281-316

This wrapper appends the change output last and then sends ReduceOutput(ffiOutputs.count - 1), but the Rust wallet does not preserve insertion indexes for fee targeting. platform_address_wallet_transfer parses the array and calls PlatformAddressWallet::transfer(), which converts outputs to a BTreeMap; the documented contract there is that output indexes are interpreted in lexicographic PlatformAddress order, not Swift insertion order. If the chosen change address sorts before a recipient, DPP will reduce a different output than Swift intended, so the broadcast transition can underpay a recipient even though Swift presented the fee as coming from change. Rust added a guard for this case in transfer_with_change_address(), but this FFI entry point still bypasses that protection.

2. Suggestion: Transfer persistence still stores external recipient addresses as if they belonged to the wallet account

packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs:121-154

This loop persists every address returned by transfer_address_funds, even though that result set includes recipient outputs as well as wallet-owned inputs. For any returned P2pkh that is not in the account's derived address pool, the code still calls set_address_credit_balance(...) and then falls back to address_index = 0 before pushing a persisted PlatformAddressBalanceEntry. On restore, insert_persisted_entry() reinserts that row into the provider's (address_index -> address) map and found balances, and initialize_from_persisted() rehydrates those persisted balances back into ManagedPlatformAccount. A transfer to an external address therefore leaks untrusted recipient data into privileged wallet state and can corrupt the restored account/address mapping.

3. Suggestion: parse_outputs still silently coalesces duplicate destination addresses

packages/rs-platform-wallet-ffi/src/platform_address_types.rs:203-205

The FFI boundary accepts an ordered output array, but this parser inserts each row into an IndexMap with map.insert(addr, entry.balance). If a non-Rust caller supplies the same destination address twice, the earlier amount is overwritten and Rust signs a different transfer than the caller constructed. Because Rust-native callers cannot express duplicate keys, this parser is the right place to reject the ambiguity instead of silently changing the spend request across the language boundary.

Suggested replacement:

        for entry in std::slice::from_raw_parts(ptr, count) {
            let addr = PlatformAddress::try_from(entry.address)?;
            if map.contains_key(&addr) {
                return Err("Duplicate output address");
            }
            map.insert(addr, entry.balance);
        }

Prior findings resolved/deferred

  • InputSelection::Auto documentation did not describe the actual supported fee strategies — Fixed in packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs: the public docs and runtime check now state that auto-selection only supports [DeductFromInput(0)] and [ReduceOutput(0)].
  • CoreChangeSet::is_empty_no_records() could drop chain-lock-only or other metadata-only rounds — Fixed in packages/rs-platform-wallet/src/changeset/core_bridge.rs: is_empty_no_records() now checks instant_locks_for_non_final_records, last_applied_chain_lock, and addresses_derived, so metadata-only persistence rounds are no longer discarded.
  • FFI collapsed OnlyOutputAddressesFunded and OnlyDustInputs into the generic unknown error path — Fixed in packages/rs-platform-wallet-ffi/src/error.rs and packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift: Rust now emits dedicated result codes 13 and 14, and Swift maps them to typed error cases.
  • Missing regression test for restore-time platform-address balance hydration — Intentionally deferred on this SHA. packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs carries TODO(CMT-004) documenting that initialize_from_persisted still lacks a direct regression test for restart-time balance hydration.

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

I revalidated the reported wallet/FFI issues against the checked-out head ceb4fc87035cef64acadeaec6f2d22af2274d7cf. All three Codex findings are real on this SHA, and the Swift transfer issue remains blocking because the wrapper still targets fee reduction by array position while the Rust/DPP path interprets ReduceOutput after lexicographic output canonicalization.

🔴 1 blocking | 🟡 2 suggestion(s)

3 additional finding(s) omitted (not in diff).

@thepastaclaw
Copy link
Copy Markdown
Collaborator

Details for the carried-forward findings in my ceb4fc87 review. GitHub could not place these as inline comments because the still-valid issues are outside the latest PR diff, so I’m putting the evidence here rather than letting the review body’s “omitted” line hide them.

  1. [blocking] Swift explicit-change transfer still targets ReduceOutput by Swift array index instead of the Rust/DPP output order

    • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:296-316
    • This wrapper appends the change output last and then sends ReduceOutput(ffiOutputs.count - 1), but the Rust transfer path does not preserve that index semantics. platform_address_wallet_transfer() parses the array, then PlatformAddressWallet::transfer() converts the outputs into a BTreeMap, and the transfer documentation explicitly states that under [ReduceOutput(0)] output indexes are interpreted by lexicographic PlatformAddress order. The newer Rust transfer_with_change_address() guard rejects lex-smaller change addresses for exactly this reason, but this Swift FFI path still calls the plain transfer entry point and bypasses that protection. If the chosen change address sorts before a recipient, the chain deducts the fee from a different output than Swift presented, so the app can underpay a recipient while claiming the fee came only from change.
  2. [suggestion] Transfer persistence still records non-wallet recipient addresses as owned account state

    • packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs:121-154
    • This loop persists every P2pkh returned in address_infos, even when the address is not derived by the wallet. It updates set_address_credit_balance() before ownership is confirmed and then fabricates address_index = 0 via unwrap_or(0) for foreign recipients. That is not a harmless cache detail: the persistence loader later calls insert_persisted_entry() with that (address_index, address) pair, and initialize_from_persisted() rehydrates every persisted found() entry back into the managed account balance map. A single transfer to an external recipient therefore poisons restored wallet state with an address the wallet cannot sign for.

    Suggested change:

                for (addr, maybe_info) in address_infos.iter() {
                    let PlatformAddress::P2pkh(hash) = addr else {
                        continue;
                    };
                    let p2pkh = PlatformP2PKHAddress::new(*hash);
                    let Some(address_index) = account
                        .addresses
                        .addresses
                        .iter()
                        .find_map(|(&idx, info)| {
                            PlatformP2PKHAddress::from_address(&info.address)
                                .ok()
                                .filter(|found| *found == p2pkh)
                                .map(|_| idx)
                        })
                    else {
                        continue;
                    };
                    let funds = match maybe_info {
                        Some(ai) => dash_sdk::platform::address_sync::AddressFunds {
                            balance: ai.balance,
                            nonce: ai.nonce,
                        },
                        None => dash_sdk::platform::address_sync::AddressFunds {
                            balance: 0,
                            nonce: 0,
                        },
                    };
                    account.set_address_credit_balance(p2pkh, funds.balance, key_source.as_ref());
                    cs.addresses.push(crate::PlatformAddressBalanceEntry {
                        wallet_id: self.wallet_id,
                        account_index,
                        address_index,
                        address: p2pkh,
                        funds,
                    });
                }
  1. [suggestion] parse_outputs still silently coalesces duplicate destination addresses at the public FFI boundary

    • packages/rs-platform-wallet-ffi/src/platform_address_types.rs:203-205
    • parse_outputs() still feeds the caller-supplied output array through map.insert(addr, entry.balance), so repeated destination rows overwrite earlier ones instead of being rejected. The Swift high-level wrapper now blocks duplicate recipients, but the exported ABI still routes every FFI caller through this parser before Rust signs the transfer. That means any direct Swift/C caller can build confirmations, totals, and change against the original array while Rust actually broadcasts the deduplicated map.

    Suggested change:

        for entry in std::slice::from_raw_parts(ptr, count) {
            let addr = PlatformAddress::try_from(entry.address)?;
            if map.contains_key(&addr) {
                return Err("Duplicate output address");
            }
            map.insert(addr, entry.balance);
        }

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Verified the reported issues against the checked-out head 6c8c5d955e2fb3b896be90fe4e27060a0b1df72d. Four Codex findings are real on this SHA: the SPV shutdown path has a lock-order deadlock, the Swift explicit-change wrapper still mis-targets ReduceOutput, and two wallet/FFI paths still persist or accept ambiguous address state. No CodeRabbit comments were provided for this run, and I did not confirm any resolved prior findings.

🔴 2 blocking | 🟡 2 suggestion(s)

4 additional finding(s) omitted (not in diff).

@thepastaclaw
Copy link
Copy Markdown
Collaborator

Code Review details for review 4356806897

Correction: GitHub accepted review 4356806897, but its body omitted the out-of-diff findings. Full details for the same checked-out head 6c8c5d955e2fb3b896be90fe4e27060a0b1df72d. Four findings are valid on this SHA: one new blocking issue in the latest delta, plus all three prior findings from ceb4fc87 remain STILL VALID and are carried forward below. No prior finding was verified as fixed, outdated, or intentionally deferred.

🔴 2 blocking | 🟡 2 suggestion(s)

Reviewed commit: 6c8c5d9

New finding in latest delta

🔴 blocking: `SpvRuntime::run()` holds the client read lock until `client.run()` returns, so `stop()` cannot ever acquire the write lock it needs

packages/rs-platform-wallet/src/spv/runtime.rs (lines 138-159)

run() reads self.client, borrows the inner client, and then awaits client.run() while the RwLockReadGuard is still alive. stop() is the only public shutdown path, and it starts by taking self.client.write().await before calling c.stop(). Once the sync loop is running, shutdown makes no forward progress: stop() waits on the write lock, while run() keeps the read lock until client.run() exits, and client.run() is exactly what stop() must terminate. The same inversion affects both the Rust test path in tests/spv_sync.rs and the FFI stop entry point in packages/rs-platform-wallet-ffi/src/spv.rs.

Carried-forward prior findings from ceb4fc87 — STILL VALID

🔴 blocking: Swift explicit-change transfers still choose `ReduceOutput` by Swift array index even though Rust/DPP reorders outputs by address

packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (lines 296-327)

This wrapper appends the change output last and sends FeeStrategyStepFFI(step_type: 1, index: ffiOutputs.count - 1), assuming the fee target is the last Swift output. That assumption is false on the Rust side. platform_address_wallet_transfer() parses the array, PlatformAddressWallet::transfer() immediately canonicalizes outputs into a BTreeMap, and the SDK/DPP path interprets ReduceOutput(0)-style indexes over that canonical order, not Swift insertion order. The Rust wallet already documents this behavior and only protects it in transfer_with_change_address(), but this Swift wrapper still calls the plain platform_address_wallet_transfer(...) entry point. If the chosen change address sorts before a recipient, the chain deducts the fee from a different output than the UI presented, so the wallet can underpay a recipient while claiming the fee came only from change.

🟡 suggestion: Post-transfer persistence still records foreign recipient addresses as wallet-owned account rows

packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (lines 121-154)

Sdk::transfer_address_funds*() returns AddressInfos for the union of input and output addresses, and this loop persists every returned P2pkh row. The code updates set_address_credit_balance(...) before proving the address belongs to the managed account, then fabricates address_index = 0 with unwrap_or(0) when the address is not in the account’s derivation pool. Those rows are later replayed through insert_persisted_entry() and initialize_from_persisted() as if they were wallet-owned state. One transfer to an external recipient is therefore enough to poison persisted account state with an address the wallet cannot sign for.

💡 Suggested change
                for (addr, maybe_info) in address_infos.iter() {
                    let PlatformAddress::P2pkh(hash) = addr else {
                        continue;
                    };
                    let p2pkh = PlatformP2PKHAddress::new(*hash);
                    let Some(address_index) = account
                        .addresses
                        .addresses
                        .iter()
                        .find_map(|(&idx, info)| {
                            PlatformP2PKHAddress::from_address(&info.address)
                                .ok()
                                .filter(|found| *found == p2pkh)
                                .map(|_| idx)
                        })
                    else {
                        continue;
                    };
                    let funds = match maybe_info {
                        Some(ai) => dash_sdk::platform::address_sync::AddressFunds {
                            balance: ai.balance,
                            nonce: ai.nonce,
                        },
                        None => dash_sdk::platform::address_sync::AddressFunds {
                            balance: 0,
                            nonce: 0,
                        },
                    };
                    account.set_address_credit_balance(p2pkh, funds.balance, key_source.as_ref());
                    cs.addresses.push(crate::PlatformAddressBalanceEntry {
                        wallet_id: self.wallet_id,
                        account_index,
                        address_index,
                        address: p2pkh,
                        funds,
                    });
                }
🟡 suggestion: `parse_outputs()` still silently overwrites duplicate destination rows at the public FFI boundary

packages/rs-platform-wallet-ffi/src/platform_address_types.rs (lines 203-205)

parse_outputs() feeds the caller-supplied array into an IndexMap with map.insert(addr, entry.balance), so a repeated PlatformAddress silently replaces the earlier amount instead of rejecting the request. Rust-native callers cannot construct duplicate keys, which makes this FFI parser the last boundary that can reject an ambiguous output set before Rust signs and broadcasts it. The Swift wrapper now rejects duplicate recipient hashes for its own call path, but this function is still exported through the public C ABI and remains reachable by other FFI callers.

💡 Suggested change
        for entry in std::slice::from_raw_parts(ptr, count) {
            let addr = PlatformAddress::try_from(entry.address)?;
            if map.contains_key(&addr) {
                return Err("Duplicate output address");
            }
            map.insert(addr, entry.balance);
        }

Prior-finding reconciliation

  • STILL VALID: Swift explicit-change ReduceOutput index mismatch.
  • STILL VALID: transfer persistence records foreign recipient addresses as wallet-owned state.
  • STILL VALID: FFI parse_outputs() silently coalesces duplicate destination rows.
  • FIXED / OUTDATED / INTENTIONALLY DEFERRED: none verified.

lklimek and others added 2 commits May 25, 2026 15:11
…ion addresses (PR #3554 CMT-003)

`parse_outputs` previously kept inserting into the output `IndexMap`
without checking for collisions, so a duplicate `AddressBalanceEntryFFI`
silently overwrote the earlier entry — the wallet would still sum every
output toward change calculation, leaving the transition misbalanced.

The Swift wrapper guards against this today (CMT-001 follow-up tracked
in #3738), but the Rust FFI is the authoritative gate. Reject duplicates
explicitly, naming the offending address hash so the caller can identify
which output collided.

Switched the return type to `Result<_, String>` and added the matching
`From<String> for PlatformWalletFFIResult` impl — the only caller goes
through `unwrap_result_or_return!` which already routes via `.into()`,
so the ripple is one new impl and one signature change.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sistence (PR #3554 CMT-002)

`transfer_address_funds` returns address info for the full
`inputs ∪ outputs` set, including external recipients the wallet does
not own. The persistence loop in `transfer` called
`set_address_credit_balance` on every returned `P2pkh` and fell back to
`address_index = 0` when the address was not in the derived pool —
persisting external recipients under a fabricated derivation index that
would poison the account's address map on restore.

Hoist the derived-pool lookup to the top of the loop, skip non-owned
addresses before touching the account, and drop the `.unwrap_or(0)`
fallback. Factor the filter + entry construction into a pure helper
`build_transfer_persistence_entries` so the invariant is unit-testable
without spinning up an SDK + wallet manager.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Claudius-Maginificent
Copy link
Copy Markdown
Collaborator Author

@thepastaclaw — addressing your three carried-forward findings from the 2026-05-22 re-validation.

CMT-002 — Transfer persistence filters external recipients (`1fba51c1`)
Refactored `transfer.rs` persistence loop to skip addresses not in the wallet's derived-address pool. The `.unwrap_or(0)` fallback that persisted external recipients under `address_index = 0` is gone. Factored the filter+entry build into pure helper `build_transfer_persistence_entries`. Two new tests: `persistence_filter_drops_external_recipients` (asserts a transfer-to-external-recipient produces no persisted entry for the recipient), `persistence_filter_treats_missing_info_as_zero`.

CMT-003 — `parse_outputs` rejects duplicates (`2eab4f7c`)
Switched `parse_outputs` to `Result<_, String>` so the error message can name the offending address. Returns `Err("Duplicate output address: ")` on collision. Single caller (`transfer.rs:41`) routes via `unwrap_result_or_return!` + `.into()` — no caller code changes. One new `From for PlatformWalletFFIResult` impl. Used `hex::encode` instead of `Debug` because `PlatformAddress::P2pkh([u8;20])`'s default Debug is a decimal byte array (unreadable). 2 new unit tests.

CMT-001 (Swift `ReduceOutput(insertion-index)` BLOCKING) — deferred to dedicated follow-up issue #3738, assigned to @shumkov. The buggy code (`changeIndex = ffiOutputs.count - 1`) is preexisting from #3626; PR #3554 doesn't touch `ManagedPlatformAddressWallet.swift` (zero diff against base). Fixing it inside #3554 would be scope creep on already-merged Swift SDK territory. The issue carries the BLOCKING severity tag and the two fix options you outlined (lex-sort outputs in Swift, OR route through `transfer_with_change_address`).

Validations: `cargo fmt --check` clean, `clippy --tests -- -D warnings` clean, `cargo test --lib` 146 passed (platform-wallet) / 80 passed (platform-wallet-ffi).

🤖 Co-authored by Claudius the Magnificent AI Agent

@thepastaclaw
Copy link
Copy Markdown
Collaborator

One remaining non-blocking concern I still see in the exact head:

auto_select_inputs() still builds its candidate set from a read-only platform-address balance snapshot, with no reservation or pre-broadcast revalidation step. The code itself calls this out around packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs:292-296:

// TODO(QA-007): pre-existing read-snapshot vs broadcast race; new
// headroom math leans harder on snapshot. Hold guard across
// snapshot+broadcast+update, or re-validate headroom right before
// broadcast.

Why it matters: a concurrent platform-address spend can invalidate the selected balances after this snapshot is taken, so the transfer path can still build from stale input state and fail later at broadcast/validation time. The new headroom logic depends more heavily on that snapshot.

Suggested fix: add a reservation/revalidation step for platform-address inputs before broadcasting, or hold the relevant wallet/account state lock across selection and transition submission.

The earlier concrete issues I checked look fixed in this head: external-recipient persistence poisoning, duplicate FFI output-address overwrites, and typed no-selectable-input errors.

@lklimek
Copy link
Copy Markdown
Contributor

lklimek commented May 25, 2026

This is a pre-existing issue,

auto_select_inputs() still builds its candidate set from a read-only platform-address balance snapshot, with no reservation or pre-broadcast revalidation step.

This is a pre-existing issue, out of scope.

@lklimek lklimek added the ready for final review Ready for the final review. If AI was involved in producing this PR, it has already had a reviewer. label May 25, 2026
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

I confirmed three real issues on head 1fba51c16c4182a0d8cb00d631a4c9883f545500. Two are blocking regressions that remain unchanged: SPV shutdown can deadlock, and the Swift transfer wrapper still computes ReduceOutput against Swift insertion order even though Rust/DPP interpret that index in lexicographic address order. I also confirmed that the duplicate-output parser bug and the foreign-recipient persistence bug are fixed in this SHA.

🔴 2 blocking | 🟡 1 suggestion(s)

3 additional finding(s) omitted (not in diff).

@thepastaclaw
Copy link
Copy Markdown
Collaborator

Code Review details for review 4357135284

Correction: the review body GitHub accepted omitted the out-of-diff finding details. I confirmed three real issues on head 1fba51c16c4182a0d8cb00d631a4c9883f545500. Two are blocking regressions that remain unchanged: SPV shutdown can deadlock, and the Swift transfer wrapper still computes ReduceOutput against Swift insertion order even though Rust/DPP interpret that index in lexicographic address order. I also confirmed that the duplicate-output parser bug and the foreign-recipient persistence bug are fixed in this SHA.

🔴 2 blocking | 🟡 1 suggestion(s)

Prior findings resolved on this SHA

  • Post-transfer persistence still records foreign recipient addresses as wallet-owned account rows — Fixed in this SHA. transfer() now builds an owned address lookup and only persists entries returned by build_transfer_persistence_entries(...), which skips addresses outside the wallet's derived pool (packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs:121-149 and 416-451).
  • parse_outputs() still silently overwrites duplicate destination rows at the public FFI boundary — Fixed in this SHA. parse_outputs() now rejects duplicate destination addresses and the unit test parse_outputs_rejects_duplicate_destination_address covers that behavior (packages/rs-platform-wallet-ffi/src/platform_address_types.rs:194-214 and 430-467).

Verified findings

blocking: SPV shutdown deadlocks because `run()` holds the read lock for the entire sync loop while `stop()` requires the write lock

Location: packages/rs-platform-wallet/src/spv/runtime.rs:138-159

SpvRuntime::run() acquires self.client.read().await and keeps that guard alive across client.run().await. SpvRuntime::stop() is the only shutdown path, but it starts by acquiring self.client.write().await before it can call c.stop(). That creates a lock inversion: once the background sync loop is running, stop() cannot obtain the write lock until run() returns, and run() cannot return until stop() signals the client to stop. The FFI stop entry point in packages/rs-platform-wallet-ffi/src/spv.rs:269-275 routes directly into this method, so foreign callers can hang indefinitely as well.

blocking: Swift explicit-change transfers target `ReduceOutput` by Swift array index even though Rust canonicalizes outputs by address before applying the fee strategy

Location: packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:296-316

This wrapper appends the change row last and then sends FeeStrategyStepFFI(step_type: 1, index: ffiOutputs.count - 1), assuming the fee target is the last Swift output. That assumption is false in the called Rust path: platform_address_wallet_transfer() parses the array, then PlatformAddressWallet::transfer() converts outputs into a BTreeMap (packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs:27-48), and its own docs state that ReduceOutput(0) refers to the lexicographically smallest output, not insertion order. If the chosen change address sorts before a recipient, the fee is deducted from a different output than Swift presented to the caller, so the broadcast transfer can underpay a recipient while claiming the fee came only from change. The new Rust-side guard in transfer_with_change_address() does not protect this path because the Swift wrapper still calls the plain platform_address_wallet_transfer(...) FFI entry point.

suggestion: The public FFI explicit-input parsers still silently overwrite duplicate rows instead of rejecting ambiguous caller input

Location: packages/rs-platform-wallet-ffi/src/platform_address_types.rs:129-162

Both parse_explicit_inputs() and parse_explicit_inputs_with_nonces() build a BTreeMap with unconditional map.insert(...). If a foreign caller passes the same address twice, the later row silently replaces the earlier balance, and in the nonce-bearing form it also replaces the earlier nonce. This is reachable from public FFI through parse_input_selection() in packages/rs-platform-wallet-ffi/src/platform_addresses/mod.rs:32-58 and is used by both transfer and withdrawal entry points. Rust-native callers cannot construct duplicate keys in these maps, so the FFI parser is the last trust boundary that can reject this ambiguity before Rust signs and broadcasts a different effective input set than the caller supplied or displayed.

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [BLOCKING] In `packages/rs-platform-wallet/src/spv/runtime.rs`:138-159: SPV shutdown deadlocks because `run()` holds the read lock for the entire sync loop while `stop()` requires the write lock
  `SpvRuntime::run()` acquires `self.client.read().await` and keeps that guard alive across `client.run().await`. `SpvRuntime::stop()` is the only shutdown path, but it starts by acquiring `self.client.write().await` before it can call `c.stop()`. That creates a lock inversion: once the background sync loop is running, `stop()` cannot obtain the write lock until `run()` returns, and `run()` cannot return until `stop()` signals the client to stop. The FFI stop entry point in `packages/rs-platform-wallet-ffi/src/spv.rs:269-275` routes directly into this method, so foreign callers can hang indefinitely as well.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift`:296-316: Swift explicit-change transfers target `ReduceOutput` by Swift array index even though Rust canonicalizes outputs by address before applying the fee strategy
  This wrapper appends the change row last and then sends `FeeStrategyStepFFI(step_type: 1, index: ffiOutputs.count - 1)`, assuming the fee target is the last Swift output. That assumption is false in the called Rust path: `platform_address_wallet_transfer()` parses the array, then `PlatformAddressWallet::transfer()` converts outputs into a `BTreeMap` (`packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs:27-48`), and its own docs state that `ReduceOutput(0)` refers to the lexicographically smallest output, not insertion order. If the chosen change address sorts before a recipient, the fee is deducted from a different output than Swift presented to the caller, so the broadcast transfer can underpay a recipient while claiming the fee came only from change. The new Rust-side guard in `transfer_with_change_address()` does not protect this path because the Swift wrapper still calls the plain `platform_address_wallet_transfer(...)` FFI entry point.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/platform_address_types.rs`:129-162: The public FFI explicit-input parsers still silently overwrite duplicate rows instead of rejecting ambiguous caller input
  Both `parse_explicit_inputs()` and `parse_explicit_inputs_with_nonces()` build a `BTreeMap` with unconditional `map.insert(...)`. If a foreign caller passes the same address twice, the later row silently replaces the earlier balance, and in the nonce-bearing form it also replaces the earlier nonce. This is reachable from public FFI through `parse_input_selection()` in `packages/rs-platform-wallet-ffi/src/platform_addresses/mod.rs:32-58` and is used by both transfer and withdrawal entry points. Rust-native callers cannot construct duplicate keys in these maps, so the FFI parser is the last trust boundary that can reject this ambiguity before Rust signs and broadcasts a different effective input set than the caller supplied or displayed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for final review Ready for the final review. If AI was involved in producing this PR, it has already had a reviewer.

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

5 participants